-
-
Notifications
You must be signed in to change notification settings - Fork 363
test(Localizer): update GetJsonStringByTypeName_UseKeyWhenValueIsNull test #7088
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR enhances the GetJsonStringByTypeName_UseKeyWhenValueIsNull unit test to handle behavior differences across .NET versions, updates expected assertion values, simplifies method calls, and refines the XML documentation for UseKeyWhenValueIsNull. Class diagram for updated JsonLocalizationOptions documentationclassDiagram
class JsonLocalizationOptions {
bool IgnoreLocalizerMissing
bool UseKeyWhenValueIsNull
}
JsonLocalizationOptions --|> LocalizationOptions
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `test/UnitTest/Utils/UtilityTest.cs:394-401` </location>
<code_context>
var test1 = items.FirstOrDefault(i => i.Name == "Test-Null");
Assert.NotNull(test1);
+
+#if NET9_0
+ Assert.Equal("", test1.Value);
+#elif NET10_0_OR_GREATER
Assert.Equal("Test-Null", test1.Value);
+#endif
var test2 = items.FirstOrDefault(i => i.Name == "Test-Key");
</code_context>
<issue_to_address>
**suggestion (testing):** Conditional assertions for different .NET versions may lead to inconsistent test coverage.
Ensure your CI pipeline runs tests for all supported .NET versions so these assertions are validated. Also, add a comment clarifying the version-specific behavior for maintainability.
```suggestion
var test1 = items.FirstOrDefault(i => i.Name == "Test-Null");
Assert.NotNull(test1);
// NOTE: The expected value for test1.Value differs by .NET version:
// - On .NET 9.0, Value is expected to be an empty string ("").
// - On .NET 10.0 or greater, Value is expected to be "Test-Null".
// Please ensure CI runs tests for all supported .NET versions so these assertions are validated.
#if NET9_0
Assert.Equal("", test1.Value);
#elif NET10_0_OR_GREATER
Assert.Equal("Test-Null", test1.Value);
#endif
```
</issue_to_address>
### Comment 2
<location> `test/UnitTest/Utils/UtilityTest.cs:405` </location>
<code_context>
var test2 = items.FirstOrDefault(i => i.Name == "Test-Key");
Assert.NotNull(test2);
- Assert.Equal("Test-Key", test2.Value);
+ Assert.Equal("", test2.Value);
option.UseKeyWhenValueIsNull = false;
</code_context>
<issue_to_address>
**suggestion (testing):** Test does not cover the scenario when both UseKeyWhenValueIsNull is false and the value is null.
Add assertions for when `UseKeyWhenValueIsNull` is false and the value is null to verify correct fallback or error handling.
</issue_to_address>
### Comment 3
<location> `test/UnitTest/Utils/UtilityTest.cs:408` </location>
<code_context>
option.UseKeyWhenValueIsNull = false;
- items = Utility.GetJsonStringByTypeName(option, this.GetType().Assembly, "UnitTest.Utils.UtilityTest", "en-US", true);
+ items = Utility.GetJsonStringByTypeName(option, GetType().Assembly, "UnitTest.Utils.UtilityTest", "en-US", true);
test1 = items.FirstOrDefault(i => i.Name == "Test-Null");
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding negative test cases for invalid or missing keys.
Please include tests for scenarios where the key is missing, or its value is null or empty, to verify correct handling of these cases.
Suggested implementation:
```csharp
test1 = items.FirstOrDefault(i => i.Name == "Test-Null");
Assert.NotNull(test1);
// Negative test: missing key
var missingKey = items.FirstOrDefault(i => i.Name == "Test-Missing");
Assert.Null(missingKey);
// Negative test: key with null value
var nullValueKey = items.FirstOrDefault(i => i.Name == "Test-ExplicitNull");
if (nullValueKey != null)
{
Assert.Null(nullValueKey.Value);
}
// Negative test: key with empty value
var emptyValueKey = items.FirstOrDefault(i => i.Name == "Test-Empty");
if (emptyValueKey != null)
{
Assert.Equal("", emptyValueKey.Value);
}
```
You may need to ensure that your test data (the JSON or source for `Utility.GetJsonStringByTypeName`) includes entries for "Test-ExplicitNull" and "Test-Empty" with appropriate values (null and empty string, respectively). If these do not exist, add them to your test data.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7088 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 745 745
Lines 32556 32556
Branches 4512 4512
=========================================
Hits 32556 32556
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR makes several improvements to the JSON localization handling, including fixing a typo in the test locale file, updating documentation, and adding framework-specific test assertions.
- Fixed typo in locale file key name (
Test-Key:→Test-Key) - Updated documentation to remove misleading exception reference
- Added conditional compilation for framework-specific test behavior
- Removed redundant
this.qualifier for code consistency
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/UnitTest/Utils/UtilityTest.cs | Added framework-specific conditional compilation for test assertions and removed redundant this. qualifier |
| test/UnitTest/Locales/en-US.json | Fixed typo in JSON key name by removing erroneous trailing colon from "Test-Key:" |
| src/BootstrapBlazor/Localization/Json/JsonLocalizationOptions.cs | Improved documentation accuracy by removing misleading exception reference |
Comments suppressed due to low confidence (2)
test/UnitTest/Utils/UtilityTest.cs:523
- Class 'MockClone' implements 'ICloneable'.
private class MockClone : ICloneable
test/UnitTest/Utils/UtilityTest.cs:511
- Field 'Field' can be 'readonly'.
public string Field = "";
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var test1 = items.FirstOrDefault(i => i.Name == "Test-Null"); | ||
| Assert.NotNull(test1); | ||
|
|
||
| #if NET9_0 |
Copilot
AI
Nov 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The conditional compilation directive uses NET9_0 (exact version match) which is inconsistent with the pattern used elsewhere in the codebase. Other files use NET9_0_OR_GREATER (e.g., in CacheManager.cs, DefaultDispatchService.cs, ICacheManager.cs).
Consider using NET9_0_OR_GREATER instead for consistency and to ensure the code works correctly for future .NET 9.x patch versions.
| #if NET9_0 | |
| #if NET9_0_OR_GREATER |
| #if NET9_0 | ||
| Assert.Equal("", test1.Value); | ||
| #elif NET10_0_OR_GREATER | ||
| Assert.Equal("Test-Null", test1.Value); | ||
| #endif |
Copilot
AI
Nov 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The NET10_0_OR_GREATER preprocessor symbol references .NET 10, which does not exist yet. As of January 2025 (my knowledge cutoff), .NET 9 is the latest stable release. While .NET 10 may be in preview or released by November 2025, this conditional compilation logic appears speculative.
Additionally, the test logic suggests different behavior between .NET 9 and .NET 10 for the same localization scenario (empty string vs. "Test-Null"), which seems unexpected. This conditional should be reviewed to ensure it reflects actual behavioral differences in the framework, or simplified if the behavior is actually the same across versions.
| #if NET9_0 | |
| Assert.Equal("", test1.Value); | |
| #elif NET10_0_OR_GREATER | |
| Assert.Equal("Test-Null", test1.Value); | |
| #endif | |
| Assert.Equal("", test1.Value); |
Link issues
fixes #7087
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Update the GetJsonStringByTypeName_UseKeyWhenValueIsNull unit test to handle different null value behaviors in .NET 9 and .NET 10+, refine related assertions and assembly call, and clarify the UseKeyWhenValueIsNull option description.
Enhancements:
Tests: